Skip to content

✨ feat(tests): EIP-7928 No-op and adjacent test case #2178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Oct 6, 2025
Merged

✨ feat(tests): EIP-7928 No-op and adjacent test case #2178

merged 15 commits into from
Oct 6, 2025

Conversation

raxhvl
Copy link
Member

@raxhvl raxhvl commented Sep 19, 2025 •
edited by fselmo
Loading

🗒️ Description

Adds No-op and no-op adjacent test cases for EIP-7928

🔗 Related Issues or PRs

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks: 
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

@raxhvl

This comment was marked as resolved.

Sign in to view
@raxhvl raxhvl added scope:tests Scope: Changes EL client test cases in `./tests` type:test Type: Add/refactor fw unit tests; no fw or el client test case changes labels Sep 19, 2025
@raxhvl raxhvl self-assigned this Sep 19, 2025
@raxhvl

This comment was marked as resolved.

Sign in to view
@raxhvl raxhvl marked this pull request as ready for review September 29, 2025 11:03
@raxhvl raxhvl force-pushed the feat/eip-7928/test-noop branch from f3ef32a to 97e7edc Compare October 1, 2025 13:04
@fselmo fselmo force-pushed the feat/eip-7928/test-noop branch 2 times, most recently from 4b4405a to ccd2966 Compare October 3, 2025 17:37
@fselmo fselmo added the type:feat type: Feature label Oct 3, 2025
@fselmo fselmo force-pushed the feat/eip-7928/test-noop branch from 85480d5 to 179c570 Compare October 3, 2025 19:50
fselmo
fselmo approved these changes Oct 3, 2025
View reviewed changes
Copy link
Collaborator

@fselmo fselmo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @raxhvl, I think this is good to go but can use your eyes as a sanity check before merging. Some things to note:

  • As we talked about, I updated the empty account expectations to use BalAccountExpectation.empty() explicitly here. We raise if ambiguous BalAccountExpectation() is used. I think this explicitness is much nicer for a reader / implementer. I added unit tests for these as well.
  • I removed a duplicate test. It looks like the fully unmutated test was exactly the same as the bal_noop_storage_write test except for checking the storage read, which I added to the latter and removed the former here.
  • I also moved the tests into the main file and added a changelog entry.

Let me know if you think this is good to go as well and we can get this merged 😄. Thanks for your work on all these 👌🏼

@fselmo fselmo force-pushed the feat/eip-7928/test-noop branch from 179c570 to 5b0d5cb Compare October 3, 2025 21:03
@fselmo fselmo force-pushed the feat/eip-7928/test-noop branch from 5b0d5cb to faed742 Compare October 4, 2025 02:25
@raxhvl
Copy link
Member Author

raxhvl commented Oct 6, 2025

I reviewed it and the tests look good.

I removed a duplicate test. It looks like the fully unmutated test was exactly the same as the bal_noop_storage_write test except for checking the storage read, which I added to the latter and removed the former

There is a difference: the fully unmutated test also includes a zero value eth transfer along with a storage no op. This means it checks two no-op operations on two account fields simultaneously (balance and storage). Since nonce and code cannot have no-op operations, this test effectively covers all that can be tested.

@fselmo
Copy link
Collaborator

fselmo commented Oct 6, 2025

the fully unmutated test also includes a zero value eth transfer along with a storage no op. This means it checks two no-op operations on two account fields simultaneously (balance and storage).

But there was no balance expectation in this test. They were exactly the same if we don't check that. I think this makes sense then. We can put it back but I will add the explicit no-balance-changes check to it as well.

@fselmo fselmo merged commit 033c89f into main Oct 6, 2025
16 checks passed
@fselmo fselmo deleted the feat/eip-7928/test-noop branch October 6, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fselmo fselmo fselmo approved these changes

Assignees

@raxhvl raxhvl

Labels
scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature type:test Type: Add/refactor fw unit tests; no fw or el client test case changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raxhvl @fselmo